src: support building with the V8 sandbox - #62237
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #62237 +/- ##
==========================================
- Coverage 90.12% 90.11% -0.02%
==========================================
Files 752 752
Lines 252315 252371 +56
Branches 47444 47453 +9
==========================================
+ Hits 227395 227420 +25
- Misses 16217 16228 +11
- Partials 8703 8723 +20
🚀 New features to boost your workflow:
|
04bfcd6 to
78e9555
Compare
addaleax
left a comment
There was a problem hiding this comment.
Reading through the diff ... why doesn't the version of ArrayBuffer::NewBackingStore() that takes a size argument use the default allocator if that's a requirement anyway? Wouldn't it be a bug in V8 if that variant of ArrayBuffer::NewBackingStore() is effectively unusable?
If we indeed need to manually reach out to the default allocator, it would probably be better to create a helper that covers all of these cases and handles the V8_ENABLE_SANDBOX switching in one location instead of those being spread out throughout the codebase
addaleax
left a comment
There was a problem hiding this comment.
This definitely fixes issues building Node.js with sandbox mode enabled, but it's still a bit unclear how this interacts with fb21829 and why specifically ArrayBuffer::NewBackingStore(isolate, size) doesn't return a backing store that is valid for the given isolate
Memory leak is only real in specific scenarios
452ab00 to
73bdead
Compare
|
This pull request has been marked as stale due to 90 days of inactivity. |
With V8_ENABLE_SANDBOX every ArrayBuffer backing store has to be allocated inside the sandbox, so memory that Node.js or a library allocated itself cannot be wrapped and has to be copied in. Several places already special-cased this, each with its own #ifdef, but a build with the sandbox enabled still failed to compile (`kMaxSafeBufferSizeForSandbox`), aborted in `crypto.randomUUID()` (`SecureBuffer` was UNREACHABLE) and threw from every caller of the malloc-owning `Buffer::New()` (v8.serialize, string transcoding, IPC, the public `Buffer::New(isolate, data, length)`), and trace_events, SEA assets and FFI still wrapped outside memory unconditionally. Add `AdoptIntoBackingStore()`, which wraps the memory as before in regular builds and copies it into an isolate-allocated backing store, running the deleter right away, when the sandbox is enabled, and use it at all of those sites so the #ifdef lives in one place. The trace category flag cannot be copied since JS polls it, so under the sandbox `getCategoryEnabledBuffer()` returns nothing and lib falls back to `isTraceCategoryEnabled()`; FFI zero-copy views throw ERR_OPERATION_FAILED there for the same reason. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
V8 defaults `v8_enable_sandbox` to on whenever the shared pointer compression cage and the external code space are enabled, and that is the configuration embedders that use the sandbox build with. Now that the sandbox builds and passes the tests, follow that default for `--experimental-pointer-compression-shared-cage` so the configuration is reachable from `configure`. Multi-cage pointer compression builds stay without it: there every IsolateGroup gets its own sandbox, and `NodeArrayBufferAllocator` always allocates from the default one. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
73bdead to
965693d
Compare
|
@addaleax @legendecas rebased and reworked - everything goes through |
With
V8_ENABLE_SANDBOXevery ArrayBuffer backing store has to be allocated inside the sandbox, so memory that Node.js or a library allocated itself can't be wrapped and has to be copied in. Several places already special-case this, each with its own#ifdef, but a sandbox build still doesn't compile (kMaxSafeBufferSizeForSandbox), aborts incrypto.randomUUID()(SecureBufferisUNREACHABLE()), throws from every caller of the malloc-owningBuffer::New()(v8.serialize(), string transcoding, IPC, the publicBuffer::New(isolate, data, length)), and trace_events, SEA assets and FFI still wrap outside memory unconditionally.Reworked since the first round to go through the isolate's allocator:
AdoptIntoBackingStore()wraps the memory as before in regular builds and, with the sandbox, copies it into a store fromArrayBuffer::NewBackingStore(isolate, ...)and runs the deleter right away. All of the sites above use it, so there is one#ifdef. Two things can't be copied since sharing the memory is the point: the trace category flag, where lib now falls back toisTraceCategoryEnabled(), and FFI'scopy: falseviews, which throwERR_OPERATION_FAILED. The second commit turns the sandbox on for--experimental-pointer-compression-shared-cage, matching V8's own default, so the configuration is reachable fromconfigure.Tests: cctest and the default, ffi, sqlite and embedding suites pass in a shared-cage sandbox build (on top of #65464, which that configuration needs to compile) with the same results as a regular build of this branch; a few tests that wrapped stack or foreign memory for convenience are adjusted, and
common.hasV8Sandboxgates the FFI zero-copy and--secure-heaptests.Disclosure: the code and this description were written by Claude Code, directed and reviewed by @codebytere.